Update number of error occurrences reported + Bug fix in HTML reporter #915
Conversation
Pull Request Test Coverage Report for Build 5449391557
💛 - Coveralls |
There was a problem hiding this comment.
@sarahsonder great work! I just left a few minor comments, in addition to my earlier feedback of adding some test cases. 😄
That said, I also realized that I said to use the JSON reporter for testing, but based on the changes you made, I'm not sure whether this reporter is actually using this configuration at all? If not, it would be great if you fixed that in this PR as well.
|
@david-yz-liu Hi Prof. Liu! I have updated the JSON reporter behaviour to limit the number of error messages and have added tests for my changes. The tests themselves work and the JSON reporter also working when I run PyTA on other files. However, the git tests are not passing due to a type error ('type' object is not subscriptable) caused by the helper function that I wrote in the JSON reporter. Could I please get some advice on how to resolve this error? Thank you! |
|
@david-yz-liu Issue fixed! I needed to use List and Dict instead of list and dict. |
python_ta/reporters/json_reporter.py
Outdated
| """Returns a list of dictionaries containing formatted error messages.""" | ||
| max_messages = self.linter.config.pyta_number_of_messages | ||
| output_lst = [] | ||
| all_msg_names = [msg.message.msg_id for msg in msgs] |
There was a problem hiding this comment.
It isn't necessary to define this separately from unique_msg_names. You can instead use a set comprehension to automatically remove duplicates.
(But also see my below comment for additional suggestions for this part.)
python_ta/reporters/json_reporter.py
Outdated
| if msg_id not in unique_msg_names | ||
| ] | ||
|
|
||
| for msg_id in unique_msg_names: |
There was a problem hiding this comment.
This code is okay but is less efficient than it could be. Instead try this approach:
- loop over just
msgs - use a
dictaccumulator mapping errormsg_idto the count of that message type "seen so far" in the loop. Note that you can initialize the accumulator using all of the unique keys (see above comprehension) with initial corresponding value 0 - then inside the loop use the accumulator to decide whether to append a message to
output_lstor not - after that loop, use a second loop over
output_lstthat sets thenumber_of_occurrencesfor each output message---we don't need to bother with updating this for messages that aren't included in the output!
|
@sarahsonder I left a review but accidentally selected "Approve" instead of "Comment". So just to be clear, please address the review comments I left and then re-request a review. Thanks! 😄 |
…e-number-of-error-occurrences-reported
for more information, see https://pre-commit.ci
…/github.com/sarahsonder/pyta into update-number-of-error-occurrences-reported
Motivation and Context
Currently, PythonTA limits the maximum number of the times the same error can shown at 5 errors. However, students have requested for all error occurrences to be shown. Furthermore, there was a bug in the HTML reporter which displayed all error occurrences despite the reporter stating that only a limited number of occurrences were being shown.
Your Changes
Description:
pyta-number-of-messagesto 0. This by default displays all instances of the same error.plain_reporterso that it limits the number of error occurrences ONLY whenpyta-number-of-messagesis not 0.html_reporterso that its behaviour mirrors that ofplain_reporter(previouslyhtml_reporterwas not limiting the number of errors being shown).pyta-number-of-messagesis not 0 and the number of error messages exceedspyta-number-of-messages.Type of change (select all that apply):
Testing
I tested my code by running both
plain_reporterandhtml_reporteron three cases:pyta-number-of-messages = 0to check that all error occurrences are being shownpyta-number-of-messages = 2and running PythonTA on a file that has 5 of the same errors. This was to check that PythonTA was limiting the number of messages shown.pyta-number-of-messages = 6and running PythonTA on a file that has 5 of the same errors. This was to check that PythonTA would still display all error occurrences.Questions and Comments
I noticed that the Configuration page in the PythonTA documentation is under construction. Is there anything that I can add to the page?
Checklist